Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.53%. Comparing base (bfc81ca) to head (d322e4d).

@@            Coverage Diff             @@
##             main   #60137      +/-   ##
==========================================
- Coverage   88.55%   88.53%   -0.02%     
==========================================
  Files         704      704              
  Lines      208087   208088       +1     
  Branches    40019    40014       -5     
==========================================
- Hits       184266   184233      -33     
- Misses      15818    15862      +44     
+ Partials     8003     7993      -10     

Could delete 2 more lines:

del 690-691, 715-716

+718 if (events.removeListener !== undefined) +719 this.emit('removeListener', type, listener);

Delete lines 690-691, 715-716, and add consolidated emit at 718-719.

The Problem I Found

When I tried this, the test failed because:

• Line 704-705: if (position < 0) return this; - early return when listener
  not found
• If we move emit to line 718, it would never execute after this early return
• But we need it to execute when removal succeeds in the array case

Current State

I kept both emit calls inside their respective branches:

• Line 692-693: emit after single listener removal
• Line 716-717: emit after array listener removal

Your consolidation proposal cannot work due to the early return at line
704-705. The emit must happen before the function can return.

Sorry if I am wrong.
if (position < 0) return this; means - if no listener found in the list, then do nothing.
Because there is no listener being removed, Not emitting 'removeListener' event is fine.
If the test failed, it should also failed before the change.

And I think

if (list === listener || list.listener === listener) {
... ...
if (events.removeListener !== undefined)
 this.emit('removeListener', type, listener);
} else if (typeof list !== 'function') {
... ...
if (events.removeListener !== undefined)
 this.emit('removeListener', type, listener);
}

is same as

if (list === listener || list.listener === listener) {
...
} else if (typeof list !== 'function') {
...
}
if (events.removeListener !== undefined)
 this.emit('removeListener', type, listener);

Sorry if I am wrong. if (position < 0) return this; means - if no listener found in the list, then do nothing. Because there is no listener being removed, Not emitting 'removeListener' event is fine. If the test failed, it should also failed before the change.

And I think

if (list === listener || list.listener === listener) {
... ...
if (events.removeListener !== undefined)
 this.emit('removeListener', type, listener);
} else if (typeof list !== 'function') {
... ...
if (events.removeListener !== undefined)
 this.emit('removeListener', type, listener);
}

is same as

if (list === listener || list.listener === listener) {
...
} else if (typeof list !== 'function') {
...
}
if (events.removeListener !== undefined)
 this.emit('removeListener', type, listener);

@simonkcleung Thank you for the suggestion! I explored consolidating the emit calls, but found a issue:

Edge Case Problem:
If both conditions fail (listener not found), the code would still emit removeListener event incorrectly.

Test case (line 42-48):

ee.on('hello', listener1);
ee.on('removeListener', common.mustNotCall());
ee.removeListener('hello', listener2);  // listener2 never added - should NOT emit

Performance:
Consolidation would require a tracking variable (let removed or let removedListener), adding overhead to every removeListener() call.

The current approach with duplicate emit calls is actually optimal:
- Zero overhead (no extra variables)
- Handles edge cases correctly
- Properly handles wrapped listeners (list.listener || listener vs listener)

I've kept the duplicate emits while fixing the original bug. Thanks for the detailed review!

@simonkcleung Thank you for the suggestion! I explored consolidating the emit calls, but found a issue:

Edge Case Problem: If both conditions fail (listener not found), the code would still emit removeListener event incorrectly.

Test case (line 42-48):

ee.on('hello', listener1);
ee.on('removeListener', common.mustNotCall());
ee.removeListener('hello', listener2);  // listener2 never added - should NOT emit

Performance:
Consolidation would require a tracking variable (let removed or let removedListener), adding overhead to every removeListener() call.

The current approach with duplicate emit calls is actually optimal:
- Zero overhead (no extra variables)
- Handles edge cases correctly
- Properly handles wrapped listeners (list.listener || listener vs listener)

I've kept the duplicate emits while fixing the original bug. Thanks for the detailed review!

You are right.